Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Optimize store #3061

Merged
merged 12 commits into from
Oct 1, 2024
Merged

chore: Optimize store #3061

merged 12 commits into from
Oct 1, 2024

Conversation

Ivansete-status
Copy link
Collaborator

@Ivansete-status Ivansete-status commented Sep 26, 2024

Description

We have three big-time consumption components in store:

  1. database
  2. communicate with database
  3. return a response to store-client

This PR enhances the point 2, because we've been applying an incorrect async approach, which was to wait for a certain state with the following technique:

while pool.isBusy():
      await sleepAsync(0.milliseconds)

or

while not dataAvailable:
  await sleepAsync(timer.milliseconds(1))

Doing that has a very big impact on performance because we were adding a massive amount of future objects to the async queue. @arnetheduck - correct me if that statement is not accurate.

Therefore, in this PR we are starting to apply a better async approach to properly coordinate tasks.


Aside from that, we are simplifying pgasyncpool.nim and suggesting a better wrapper for the DbConn object.


We are also optimizing the following query so that we use the messages_lookup table instead:
SELECT timestamp FROM messages WHERE messageHash = $1

Copy link

This PR may contain changes to database schema of one of the drivers.

If you are introducing any changes to the schema, make sure the upgrade from the latest release to this change passes without any errors/issues.

Please make sure the label release-notes is added to make sure upgrade instructions properly highlight this change.

Copy link

github-actions bot commented Sep 26, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3061

Built from 19af1f1

@Ivansete-status Ivansete-status force-pushed the optimize-store-queries branch 2 times, most recently from c6f3249 to d3a33da Compare September 28, 2024 15:39
@Ivansete-status Ivansete-status changed the title chore: [WIP] Optimize store queries chore: Optimize store Sep 30, 2024
@Ivansete-status Ivansete-status marked this pull request as ready for review September 30, 2024 11:39
Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks so much!

Quick question - does doing

await sleepAsync(0.milliseconds)

pollute the pending Futures?

My understanding is that it gives control back to the event loop and the sleep's future is cleaned promptly, there shouldn't be more than one instance of the sleepAsync(0.milliseconds) in the pending Futures at once. But maybe I'm wrong

waku/common/databases/db_postgres/dbconn.nim Outdated Show resolved Hide resolved
@Ivansete-status
Copy link
Collaborator Author

LGTM, thanks so much!

Quick question - does doing

await sleepAsync(0.milliseconds)

pollute the pending Futures?

My understanding is that it gives control back to the event loop and the sleep's future is cleaned promptly, there shouldn't be more than one instance of the sleepAsync(0.milliseconds) in the pending Futures at once. But maybe I'm wrong

Yes I understood it the same way but after some analysis, I got the impression that it doesn't work exactly that way

Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Great work!

pool.conns[i].busy = false

const SlowQueryThresholdInNanoSeconds = 2_000_000_000
const SlowQueryThresholdInNanoSeconds = 1_000_000_000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a hint, you may like it.
const SlowQueryThresholdInNanoSeconds = 1.seconds
... and use it if needed like...
SlowQueryThresholdInNanoSeconds.nanos()

I feel it more readable instead of counting the zeros :-)
WDYT would it work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's much better that way thanks! I've applied the suggestion in 54e635d

@Ivansete-status Ivansete-status merged commit 5875ed6 into master Oct 1, 2024
9 of 11 checks passed
@Ivansete-status Ivansete-status deleted the optimize-store-queries branch October 1, 2024 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants